feat: separate manually added bookmarks from RSS subscriptions in UI and rules#2549
feat: separate manually added bookmarks from RSS subscriptions in UI and rules#2549VedantMadane wants to merge 5 commits intokarakeep-app:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds bookmark source awareness across the app: UI shows source indicators, the rule system gains a new bookmarkSourceIs condition with types and translations, and the crawler worker adds richer asset tracking and cleanup on failures. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/components/dashboard/bookmarks/BookmarkLayoutAdaptingCard.tsx`:
- Around line 99-105: The tooltip always uses t("common.bookmark_sources.web")
for all manualSources, causing extension/mobile/cli to be labeled "Web"; update
the title logic in BookmarkLayoutAdaptingCard so it derives the correct
translation key from bookmark.source (e.g. map or compute
`common.bookmark_sources.${bookmark.source}` or use a small switch) instead of
hardcoding "web", and fall back to a generic label if the source key is missing;
adjust the title prop on the div where manualSources is checked to use that
dynamic translation.
- Around line 414-416: SourceIndicator is positioned in the top-right where
owner and hover action overlays also render, causing overlap; in
BookmarkLayoutAdaptingCard change its wrapper positioning (the div containing
<SourceIndicator bookmark={bookmark} />) to a non-conflicting corner or offset
(for example replace "absolute right-2 top-2 z-40" with "absolute left-2 top-2
z-40" or "absolute right-8 top-2 z-40") so it no longer overlaps owner/hover
indicators, and run/update any visual/snapshot tests affected by the layout
change.
In `@apps/workers/workers/crawlerWorker.ts`:
- Around line 1559-1619: The catch block unconditionally deletes
downloadedAssetId even if the DB transaction committed and the asset is now
live; modify handleAsAssetBookmark so cleanup only runs when the transaction did
not commit: after calling downloadAndStoreFile set downloadedAssetId, introduce
a boolean like transactionCommitted (default false), set it to true at the end
of the db.transaction callback (after updateAsset/insert/update/delete), then
call AssetPreprocessingQueue.enqueue; in the catch, only call
silentDeleteAsset(userId, downloadedAssetId) if !transactionCommitted (i.e., the
DB changes never committed). Reference symbols: downloadedAssetId,
downloadAndStoreFile, db.transaction, AssetPreprocessingQueue.enqueue,
silentDeleteAsset.
- Around line 1836-1848: The code races storeScreenshot/storePdf against
abortPromise but doesn’t cancel the underlying storage calls, which can complete
and create assets that never get tracked; modify the storage flow so storage is
abortable and/or cleaned up: update storeScreenshot and storePdf to accept an
AbortSignal (or return a handle) and have them abort any in-flight upload when
abortSignal is triggered, or after the storage promise resolves check
abortSignal.aborted and if aborted call the corresponding cleanup/delete
function for the returned asset before returning/adding to newAssetIds; ensure
you only push screenshotAssetInfo.assetId / pdfAssetInfo.assetId into
newAssetIds after confirming the operation wasn’t aborted (use
abortSignal.throwIfAborted or signal.aborted) and wire abortSignal into the
Promise.race path so no orphaned assets remain.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/components/dashboard/bookmarks/BookmarkLayoutAdaptingCard.tsxapps/web/components/dashboard/rules/RuleEngineConditionBuilder.tsxapps/web/lib/i18n/locales/en/translation.jsonapps/workers/workers/crawlerWorker.tspackages/shared/types/rules.tspackages/trpc/lib/ruleEngine.ts
apps/web/components/dashboard/bookmarks/BookmarkLayoutAdaptingCard.tsx
Outdated
Show resolved
Hide resolved
| let downloadedAssetId: string | undefined; | ||
| try { | ||
| const downloaded = await downloadAndStoreFile( | ||
| url, | ||
| userId, | ||
| jobId, | ||
| assetType, | ||
| abortSignal, | ||
| ); | ||
| if (!downloaded) { | ||
| return; | ||
| } | ||
| downloadedAssetId = downloaded.assetId; | ||
| const fileName = path.basename(new URL(url).pathname); | ||
| await db.transaction(async (trx) => { | ||
| await updateAsset( | ||
| undefined, | ||
| { | ||
| id: downloaded.assetId, | ||
| bookmarkId, | ||
| userId, | ||
| assetType: AssetTypes.BOOKMARK_ASSET, | ||
| contentType: downloaded.contentType, | ||
| size: downloaded.size, | ||
| fileName, | ||
| }, | ||
| trx, | ||
| ); | ||
| await trx.insert(bookmarkAssets).values({ | ||
| id: bookmarkId, | ||
| assetType, | ||
| assetId: downloaded.assetId, | ||
| content: null, | ||
| fileName, | ||
| sourceUrl: url, | ||
| }); | ||
| // Switch the type of the bookmark from LINK to ASSET | ||
| await trx | ||
| .update(bookmarks) | ||
| .set({ type: BookmarkTypes.ASSET }) | ||
| .where(eq(bookmarks.id, bookmarkId)); | ||
| await trx.delete(bookmarkLinks).where(eq(bookmarkLinks.id, bookmarkId)); | ||
| }); | ||
| await AssetPreprocessingQueue.enqueue( | ||
| { | ||
| id: downloaded.assetId, | ||
| bookmarkId, | ||
| userId, | ||
| assetType: AssetTypes.BOOKMARK_ASSET, | ||
| contentType: downloaded.contentType, | ||
| size: downloaded.size, | ||
| fileName, | ||
| fixMode: false, | ||
| }, | ||
| { | ||
| groupId: userId, | ||
| }, | ||
| trx, | ||
| ); | ||
| await trx.insert(bookmarkAssets).values({ | ||
| id: bookmarkId, | ||
| assetType, | ||
| assetId: downloaded.assetId, | ||
| content: null, | ||
| fileName, | ||
| sourceUrl: url, | ||
| }); | ||
| // Switch the type of the bookmark from LINK to ASSET | ||
| await trx | ||
| .update(bookmarks) | ||
| .set({ type: BookmarkTypes.ASSET }) | ||
| .where(eq(bookmarks.id, bookmarkId)); | ||
| await trx.delete(bookmarkLinks).where(eq(bookmarkLinks.id, bookmarkId)); | ||
| }); | ||
| await AssetPreprocessingQueue.enqueue( | ||
| { | ||
| bookmarkId, | ||
| fixMode: false, | ||
| }, | ||
| { | ||
| groupId: userId, | ||
| }, | ||
| ); | ||
| } catch (error) { | ||
| if (downloadedAssetId) { | ||
| logger.error( | ||
| `[Crawler][${jobId}] handleAsAssetBookmark encountered an error, cleaning up new asset ${downloadedAssetId}: ${error}`, | ||
| ); | ||
| await silentDeleteAsset(userId, downloadedAssetId); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Post-commit enqueue failure can delete a committed asset.
downloadedAssetId cleanup in the catch block runs even when the DB transaction already committed (Line 1573-1601). If enqueue fails afterward, this deletes live data and leaves the converted bookmark inconsistent.
🔧 Suggested fix
- let downloadedAssetId: string | undefined;
+ let downloadedAssetId: string | undefined;
+ let conversionCommitted = false;
try {
const downloaded = await downloadAndStoreFile(
url,
userId,
jobId,
assetType,
abortSignal,
);
if (!downloaded) {
return;
}
downloadedAssetId = downloaded.assetId;
const fileName = path.basename(new URL(url).pathname);
await db.transaction(async (trx) => {
await updateAsset(
undefined,
{
id: downloaded.assetId,
bookmarkId,
userId,
assetType: AssetTypes.BOOKMARK_ASSET,
contentType: downloaded.contentType,
size: downloaded.size,
fileName,
},
trx,
);
await trx.insert(bookmarkAssets).values({
id: bookmarkId,
assetType,
assetId: downloaded.assetId,
content: null,
fileName,
sourceUrl: url,
});
await trx
.update(bookmarks)
.set({ type: BookmarkTypes.ASSET })
.where(eq(bookmarks.id, bookmarkId));
await trx.delete(bookmarkLinks).where(eq(bookmarkLinks.id, bookmarkId));
});
+ conversionCommitted = true;
- await AssetPreprocessingQueue.enqueue(
- {
- bookmarkId,
- fixMode: false,
- },
- {
- groupId: userId,
- },
- );
+ await AssetPreprocessingQueue.enqueue(
+ { bookmarkId, fixMode: false },
+ { groupId: userId },
+ );
} catch (error) {
- if (downloadedAssetId) {
+ if (!conversionCommitted && downloadedAssetId) {
logger.error(
`[Crawler][${jobId}] handleAsAssetBookmark encountered an error, cleaning up new asset ${downloadedAssetId}: ${error}`,
);
await silentDeleteAsset(userId, downloadedAssetId);
}
throw error;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let downloadedAssetId: string | undefined; | |
| try { | |
| const downloaded = await downloadAndStoreFile( | |
| url, | |
| userId, | |
| jobId, | |
| assetType, | |
| abortSignal, | |
| ); | |
| if (!downloaded) { | |
| return; | |
| } | |
| downloadedAssetId = downloaded.assetId; | |
| const fileName = path.basename(new URL(url).pathname); | |
| await db.transaction(async (trx) => { | |
| await updateAsset( | |
| undefined, | |
| { | |
| id: downloaded.assetId, | |
| bookmarkId, | |
| userId, | |
| assetType: AssetTypes.BOOKMARK_ASSET, | |
| contentType: downloaded.contentType, | |
| size: downloaded.size, | |
| fileName, | |
| }, | |
| trx, | |
| ); | |
| await trx.insert(bookmarkAssets).values({ | |
| id: bookmarkId, | |
| assetType, | |
| assetId: downloaded.assetId, | |
| content: null, | |
| fileName, | |
| sourceUrl: url, | |
| }); | |
| // Switch the type of the bookmark from LINK to ASSET | |
| await trx | |
| .update(bookmarks) | |
| .set({ type: BookmarkTypes.ASSET }) | |
| .where(eq(bookmarks.id, bookmarkId)); | |
| await trx.delete(bookmarkLinks).where(eq(bookmarkLinks.id, bookmarkId)); | |
| }); | |
| await AssetPreprocessingQueue.enqueue( | |
| { | |
| id: downloaded.assetId, | |
| bookmarkId, | |
| userId, | |
| assetType: AssetTypes.BOOKMARK_ASSET, | |
| contentType: downloaded.contentType, | |
| size: downloaded.size, | |
| fileName, | |
| fixMode: false, | |
| }, | |
| { | |
| groupId: userId, | |
| }, | |
| trx, | |
| ); | |
| await trx.insert(bookmarkAssets).values({ | |
| id: bookmarkId, | |
| assetType, | |
| assetId: downloaded.assetId, | |
| content: null, | |
| fileName, | |
| sourceUrl: url, | |
| }); | |
| // Switch the type of the bookmark from LINK to ASSET | |
| await trx | |
| .update(bookmarks) | |
| .set({ type: BookmarkTypes.ASSET }) | |
| .where(eq(bookmarks.id, bookmarkId)); | |
| await trx.delete(bookmarkLinks).where(eq(bookmarkLinks.id, bookmarkId)); | |
| }); | |
| await AssetPreprocessingQueue.enqueue( | |
| { | |
| bookmarkId, | |
| fixMode: false, | |
| }, | |
| { | |
| groupId: userId, | |
| }, | |
| ); | |
| } catch (error) { | |
| if (downloadedAssetId) { | |
| logger.error( | |
| `[Crawler][${jobId}] handleAsAssetBookmark encountered an error, cleaning up new asset ${downloadedAssetId}: ${error}`, | |
| ); | |
| await silentDeleteAsset(userId, downloadedAssetId); | |
| } | |
| throw error; | |
| } | |
| let downloadedAssetId: string | undefined; | |
| let conversionCommitted = false; | |
| try { | |
| const downloaded = await downloadAndStoreFile( | |
| url, | |
| userId, | |
| jobId, | |
| assetType, | |
| abortSignal, | |
| ); | |
| if (!downloaded) { | |
| return; | |
| } | |
| downloadedAssetId = downloaded.assetId; | |
| const fileName = path.basename(new URL(url).pathname); | |
| await db.transaction(async (trx) => { | |
| await updateAsset( | |
| undefined, | |
| { | |
| id: downloaded.assetId, | |
| bookmarkId, | |
| userId, | |
| assetType: AssetTypes.BOOKMARK_ASSET, | |
| contentType: downloaded.contentType, | |
| size: downloaded.size, | |
| fileName, | |
| }, | |
| trx, | |
| ); | |
| await trx.insert(bookmarkAssets).values({ | |
| id: bookmarkId, | |
| assetType, | |
| assetId: downloaded.assetId, | |
| content: null, | |
| fileName, | |
| sourceUrl: url, | |
| }); | |
| // Switch the type of the bookmark from LINK to ASSET | |
| await trx | |
| .update(bookmarks) | |
| .set({ type: BookmarkTypes.ASSET }) | |
| .where(eq(bookmarks.id, bookmarkId)); | |
| await trx.delete(bookmarkLinks).where(eq(bookmarkLinks.id, bookmarkId)); | |
| }); | |
| conversionCommitted = true; | |
| await AssetPreprocessingQueue.enqueue( | |
| { bookmarkId, fixMode: false }, | |
| { groupId: userId }, | |
| ); | |
| } catch (error) { | |
| if (!conversionCommitted && downloadedAssetId) { | |
| logger.error( | |
| `[Crawler][${jobId}] handleAsAssetBookmark encountered an error, cleaning up new asset ${downloadedAssetId}: ${error}`, | |
| ); | |
| await silentDeleteAsset(userId, downloadedAssetId); | |
| } | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workers/workers/crawlerWorker.ts` around lines 1559 - 1619, The catch
block unconditionally deletes downloadedAssetId even if the DB transaction
committed and the asset is now live; modify handleAsAssetBookmark so cleanup
only runs when the transaction did not commit: after calling
downloadAndStoreFile set downloadedAssetId, introduce a boolean like
transactionCommitted (default false), set it to true at the end of the
db.transaction callback (after updateAsset/insert/update/delete), then call
AssetPreprocessingQueue.enqueue; in the catch, only call
silentDeleteAsset(userId, downloadedAssetId) if !transactionCommitted (i.e., the
DB changes never committed). Reference symbols: downloadedAssetId,
downloadAndStoreFile, db.transaction, AssetPreprocessingQueue.enqueue,
silentDeleteAsset.
| const screenshotAssetInfo = await Promise.race([ | ||
| storeScreenshot(screenshot, userId, jobId), | ||
| abortPromise(abortSignal), | ||
| ]); | ||
| if (screenshotAssetInfo) { | ||
| await updateAsset( | ||
| oldScreenshotAssetId, | ||
| { | ||
| id: screenshotAssetInfo.assetId, | ||
| bookmarkId, | ||
| userId, | ||
| assetType: AssetTypes.LINK_SCREENSHOT, | ||
| contentType: screenshotAssetInfo.contentType, | ||
| size: screenshotAssetInfo.size, | ||
| fileName: screenshotAssetInfo.fileName, | ||
| }, | ||
| txn, | ||
| ); | ||
| assetDeletionTasks.push( | ||
| silentDeleteAsset(userId, oldScreenshotAssetId), | ||
| ); | ||
| newAssetIds.push(screenshotAssetInfo.assetId); | ||
| } | ||
| abortSignal.throwIfAborted(); | ||
|
|
||
| const pdfAssetInfo = await Promise.race([ | ||
| storePdf(pdf, userId, jobId), | ||
| abortPromise(abortSignal), | ||
| ]); |
There was a problem hiding this comment.
Abort racing can leave untracked assets behind.
Promise.race here does not cancel storeScreenshot/storePdf. If abort wins first, the storage promise can still complete and persist assets whose IDs never get added to newAssetIds.
🔧 Suggested fix
- const screenshotAssetInfo = await Promise.race([
- storeScreenshot(screenshot, userId, jobId),
- abortPromise(abortSignal),
- ]);
+ const screenshotAssetInfo = await storeScreenshot(
+ screenshot,
+ userId,
+ jobId,
+ );
+ abortSignal.throwIfAborted();
if (screenshotAssetInfo) {
newAssetIds.push(screenshotAssetInfo.assetId);
}
- abortSignal.throwIfAborted();
- const pdfAssetInfo = await Promise.race([
- storePdf(pdf, userId, jobId),
- abortPromise(abortSignal),
- ]);
+ const pdfAssetInfo = await storePdf(pdf, userId, jobId);
+ abortSignal.throwIfAborted();
if (pdfAssetInfo) {
newAssetIds.push(pdfAssetInfo.assetId);
}
- abortSignal.throwIfAborted();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workers/workers/crawlerWorker.ts` around lines 1836 - 1848, The code
races storeScreenshot/storePdf against abortPromise but doesn’t cancel the
underlying storage calls, which can complete and create assets that never get
tracked; modify the storage flow so storage is abortable and/or cleaned up:
update storeScreenshot and storePdf to accept an AbortSignal (or return a
handle) and have them abort any in-flight upload when abortSignal is triggered,
or after the storage promise resolves check abortSignal.aborted and if aborted
call the corresponding cleanup/delete function for the returned asset before
returning/adding to newAssetIds; ensure you only push
screenshotAssetInfo.assetId / pdfAssetInfo.assetId into newAssetIds after
confirming the operation wasn’t aborted (use abortSignal.throwIfAborted or
signal.aborted) and wire abortSignal into the Promise.race path so no orphaned
assets remain.
Greptile SummaryThis PR adds the ability to distinguish bookmark sources (RSS vs. manual) both visually (new Key issues identified:
The core bookmark-source feature (types, rule engine, UI indicators) is correctly implemented and low-risk. Confidence Score: 2/5
Last reviewed commit: f30e4f5 |
apps/web/components/dashboard/bookmarks/BookmarkLayoutAdaptingCard.tsx
Outdated
Show resolved
Hide resolved
apps/web/components/dashboard/rules/RuleEngineConditionBuilder.tsx
Outdated
Show resolved
Hide resolved
apps/web/components/dashboard/bookmarks/BookmarkLayoutAdaptingCard.tsx
Outdated
Show resolved
Hide resolved
….tsx Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…Card.tsx Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Made-with: Cursor
|
@greptile review |
| "and": "All of the following are true", | ||
| "or": "Any of the following are true" | ||
| "bookmark_source_is": "Bookmark Source Is", | ||
| "and": "All of the following are true", "or": "Any of the following are true" |
There was a problem hiding this comment.
Formatting error: two entries merged onto one line.
A newline was lost between "and" and "or", leaving both JSON entries on the same line. While technically valid JSON, this is clearly unintentional and inconsistent with the rest of the file's formatting.
| "and": "All of the following are true", "or": "Any of the following are true" | |
| "and": "All of the following are true", | |
| "or": "Any of the following are true" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/lib/i18n/locales/en/translation.json
Line: 384
Comment:
Formatting error: two entries merged onto one line.
A newline was lost between `"and"` and `"or"`, leaving both JSON entries on the same line. While technically valid JSON, this is clearly unintentional and inconsistent with the rest of the file's formatting.
```suggestion
"and": "All of the following are true",
"or": "Any of the following are true"
```
How can I resolve this? If you propose a fix, please make it concise.| }); | ||
|
|
||
| // Delete the old assets if any | ||
| await Promise.all(assetDeletionTasks); |
There was a problem hiding this comment.
newAssetIds not cleared after transaction commits, risking deletion of live assets
Once the db.transaction(...) at line 1893 completes successfully, all entries in newAssetIds are now referenced by the database. However, newAssetIds is never cleared after the transaction. If the subsequent await Promise.all(assetDeletionTasks) throws (e.g., if silentDeleteAsset rejects), the catch block at line 2020 will iterate over newAssetIds and call silentDeleteAsset on every one of them — deleting assets that are already committed and referenced in the DB, leaving dangling references.
The fix is to clear newAssetIds immediately after the transaction succeeds, so the catch handler only ever deletes uncommitted assets:
| }); | |
| // Delete the old assets if any | |
| await Promise.all(assetDeletionTasks); | |
| }); | |
| newAssetIds.length = 0; // Assets are now committed; don't clean them up on error | |
| // Delete the old assets if any | |
| await Promise.all(assetDeletionTasks); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/workers/workers/crawlerWorker.ts
Line: 1968-1971
Comment:
`newAssetIds` not cleared after transaction commits, risking deletion of live assets
Once the `db.transaction(...)` at line 1893 completes successfully, all entries in `newAssetIds` are now referenced by the database. However, `newAssetIds` is never cleared after the transaction. If the subsequent `await Promise.all(assetDeletionTasks)` throws (e.g., if `silentDeleteAsset` rejects), the catch block at line 2020 will iterate over `newAssetIds` and call `silentDeleteAsset` on every one of them — deleting assets that are already committed and referenced in the DB, leaving dangling references.
The fix is to clear `newAssetIds` immediately after the transaction succeeds, so the catch handler only ever deletes uncommitted assets:
```suggestion
});
newAssetIds.length = 0; // Assets are now committed; don't clean them up on error
// Delete the old assets if any
await Promise.all(assetDeletionTasks);
```
How can I resolve this? If you propose a fix, please make it concise.|
Thanks for the PR! it seem that this PR is mixing some content from the other PR about asset cleanup? Mind decoupling them so that we can merge them separately ? |
|
Also, it seems that the main improvement was also implemented in #2547. So closing this PR for now. |
Fixes #2526.
Summary
This PR adds the ability to distinguish between manually added bookmarks and those received via RSS subscriptions, both visually and through automated rules.
Key Changes: